-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SM-118 Staking Power #161
SM-118 Staking Power #161
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, mostly ready to me!
contracts/staking/StakingManager.sol
Outdated
|
||
function initialize( | ||
IERC20 token, | ||
RewardsManager rewardsManager, | ||
EpochsManager epochsManager, | ||
ISeekerPowerOracle seekerPowerOracle, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, is it inconsistent that we take other contracts as classes, but take the oracle as interface? Should we do the same for all of them?
contracts/staking/StakingManager.sol
Outdated
@@ -17,6 +18,10 @@ import "../interfaces/staking/IStakingManager.sol"; | |||
* and delegated stakers are rewarded on a pro-rata basis. | |||
*/ | |||
contract StakingManager is IStakingManager, Initializable, Ownable2StepUpgradeable, ERC165 { | |||
// The maximum possible SYLO that exists in the network. Naturally | |||
// represents the maximum possible SYLO that can be staked. | |||
uint256 internal constant MAX_SYLO = 10_000_000_000 ether; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this contract imports Utils
, should we import this constant from Utils
?
test/staking.test.ts
Outdated
).to.be.revertedWithCustomError(directory, 'NoStakeToJoinEpoch'); | ||
}); | ||
|
||
it('cannot join directory with invalid arguments', async () => { | ||
await directory.addManager(owner); | ||
await expect( | ||
directory.joinNextDirectory(ethers.ZeroAddress), | ||
directory.joinNextDirectory(ethers.ZeroAddress, 1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
directory.joinNextDirectory(ethers.ZeroAddress, 1), | |
directory.joinNextDirectory(ethers.ZeroAddress, defaultSeekerId), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you miss this comment?
test/staking.test.ts
Outdated
}); | ||
|
||
it('returns maximum SYLO amount if seeker power is very large', async () => { | ||
const maxSylo = ethers.parseEther('10000000000'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we create a constant for this?
test/staking.test.ts
Outdated
const maxSylo = ethers.parseEther('10000000000'); | ||
|
||
// 1 more than the maximum sylo | ||
await seekerPowerOracle.registerSeekerPowerRestricted(111, maxSylo + 1n); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is not needed when the below we have tested it with Math.sqrt
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also tested because we want to avoid multiplying the power if it would cause an overflow
test/staking.test.ts
Outdated
it('joins directory with stake where maximum is dependent on seeker power', async () => { | ||
const stakeToAdd = ethers.parseEther('1000000'); | ||
|
||
await token.approve(stakingManager.getAddress(), stakeToAdd); | ||
await stakingManager.addStake(stakeToAdd, owner); | ||
|
||
// the added stake is 1,000,000 SYLO, but the seeker power capacity | ||
// is 490,000 SYLO. | ||
await seekerPowerOracle.registerSeekerPowerRestricted(111, 700); | ||
|
||
await directory.addManager(owner); | ||
await directory.joinNextDirectory(owner, 111); | ||
|
||
const joinedStake = await directory.getTotalStakeForStakee(1, owner); | ||
|
||
expect(joinedStake).to.equal(ethers.parseEther('490000')); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is very niceeeee 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, should we have a test case that the stake amount is effected by both of the seekerCapaciity
and stakingProportion
?
Btw, I don't see any deployment and ganache contracts update in this PR. Is it missing the last commit? |
d96cae6
to
f3a0e2f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely!
function setDefaultPayoutPercentage(uint16 _defaultPayoutPercentage) external onlyOwner { | ||
if (_defaultPayoutPercentage > 10000) { | ||
revert PercentageCannotExceed10000(); | ||
function setDefaultPayoutPercentage(uint32 _defaultPayoutPercentage) external onlyOwner { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's update the comment above too. Also comments in percOf
, asPerc
, setDecayRate
, minimumStakeProportion
.
* denominator is 100000.
// If the Seeker Power is already | ||
// at the maximum sylo, then we just return the max sylo value directly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// If the Seeker Power is already | |
// at the maximum sylo, then we just return the max sylo value directly. | |
// If the Seeker Power is already at the maximum sylo, then we just | |
// return the max sylo value directly. |
case 'porcini-testing': | ||
return configs.PorciniTestingParameters; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we remove config testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We just haven't been using it. Adds confusion if you select the wrong one.
{ | ||
name: ContractNames.seekerPowerOracle, | ||
args: [config.SeekerPowerOracle.oracleAccount], | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this a precompiled contract like Seekers
or SyloToken
, or it will belong to our contract set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is one of our deployed contracts
"seekers": "0x49C537a88016186Ef41713239799Fc975F9e9aFA", | ||
"seekerPowerOracle": "0xca7efb9aA54e70F7a8e7efb878A48BaefA34F4AC", | ||
"futurepassRegistrar": "0x7DBf77bb534997892e5Bcfbdc79Dd82E71C35245" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add the last line here?
deployments/genesis.config.ts
Outdated
}, | ||
|
||
SeekerPowerOracle: { | ||
oracleAccount: '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this can be empty, should we check for zero address here?
deployments/genesis.config.ts
Outdated
}, | ||
|
||
SeekerPowerOracle: { | ||
oracleAccount: '0x835dF5fE77D479695a616F79A3FC3a25310eb7c6', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oracleAccount: '0x835dF5fE77D479695a616F79A3FC3a25310eb7c6', | |
oracleAccount: '0x835dF5fE77D479695a616F79A3FC3a25310eb7c6', // deployer |
test/staking.test.ts
Outdated
).to.be.revertedWithCustomError(directory, 'NoStakeToJoinEpoch'); | ||
}); | ||
|
||
it('cannot join directory with invalid arguments', async () => { | ||
await directory.addManager(owner); | ||
await expect( | ||
directory.joinNextDirectory(ethers.ZeroAddress), | ||
directory.joinNextDirectory(ethers.ZeroAddress, 1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you miss this comment?
|
||
const joinedStake = await directory.getTotalStakeForStakee(1, owner); | ||
|
||
expect(joinedStake).to.equal(ethers.parseEther('4000000')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking, should we have an event when the staking amount is deducted, so that we or user can query it when needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already possible to see the amount of stake the node joined the directory with
if (totalStake > seekerStakingCapacity && totalStake > minProportionStakingCapacity) { | ||
joiningStake = Math.min(seekerStakingCapacity, minProportionStakingCapacity); | ||
} else if (totalStake > seekerStakingCapacity) { | ||
joiningStake = seekerStakingCapacity; | ||
} else if (totalStake > minProportionStakingCapacity) { | ||
joiningStake = minProportionStakingCapacity; | ||
} else { // uncapped |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use switch case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is a switch case in solidity?
ea9c164
to
aa13035
Compare
aa13035
to
ea6f0cf
Compare
The changes in this PR include using the SeekerPowerOracle to determine a Seeker's staking capacity. It introduces a new method to the staking manager
calculateCapacityFromSeekerPower
that first queries a Seeker's power level then uses that value to determine it's staking capacity. The current algorithm is as follows:Seeker power multipler is a constant that we can change through a setter. This will give us flexibility post deployment.
The Directory contract has been updated to call
calculateCapacityFromSeekerPower
when a node joins an epoch. The maximum amount of stake the node can join an epoch with will also be determined by the staking capacity value. The maximum joined stake cannot exceed either seeker stake cap, or the minimum stake proportion stake. However for the next release, we will set the min staking proportion value to a very low number (1/10000). This requires a change in the percentage denominator to go from a uint16 value to a uint32 value.The deployment script has also been updated to support deploying the SeekerPowerOracle contract.